Implementing Resilience #3

hermeswaldemarin · 2023-03-07T00:10:11Z

Implementing Resilience and Cache Layers in Java SDK

marcio-absmartly · 2023-03-27T09:20:47Z

build.gradle

@@ -13,14 +13,16 @@ plugins {


 ext {
-	VERSION = "1.5.3"
+	VERSION = "1.5.4"


I think we should bump minor version here to 1.6.0.

marcio-absmartly · 2023-03-27T09:28:31Z

core-api/src/main/java/com/absmartly/sdk/ResilientContextDataProvider.java

+				.whenComplete(new BiConsumer<ContextData, Throwable>() {
+					@Override
+					public void accept(ContextData contextData, Throwable throwable) {
+						if (throwable == null


What happens if throwable is not null?

Nothing happens, just follow the stream calls and will fall in to the exceptionally. I'll check if the test validate this point, but I think yes.

If throwable is null nothing happen in this code, the call will be sent to exceptionally step.

marcio-absmartly · 2023-03-27T09:30:34Z

core-api/src/main/java/com/absmartly/sdk/ResilientContextDataProvider.java

+						if (contextData != null)
+							return contextData;
+
+						throw (CompletionException) throwable;


This cast is safe because all exceptions thrown the executions through CompletableFuture are wrapped in a CompletionException, right?

The internal code always throw the exception with dataFuture.completeExceptionally. This will always return a CompletionException instance

marcio-absmartly · 2023-03-27T09:31:57Z

core-api/src/main/java/com/absmartly/sdk/ResilientContextEventHandler.java

+
+	public void flushCache() {
+		List<PublishEvent> events = localCache.retrieveEvents();
+		System.out.println("Sending events in cache: " + events.size());


Leftover log?

Yep deleted.

marcio-absmartly · 2023-03-27T09:34:59Z

core-api/src/main/java/com/absmartly/sdk/cache/LocalCache.java

+import com.absmartly.sdk.json.PublishEvent;
+
+public interface LocalCache {
+	void writeEvent(PublishEvent event);


Maybe call these writePublishEvent and retrievePublishEvents?

marcio-absmartly · 2023-03-27T09:45:22Z

core-api/src/main/java/com/absmartly/sdk/cache/AbstractCache.java

+	private final ObjectMapper mapper;
+
+	public AbstractCache() {
+		final ObjectMapper objectMapper = new ObjectMapper();


I think we should use the already existing interfaces here ContextEventSerializer and ContextDataSerializer.
The reason is that some users might not want to use Jackson, but use Gson or whatever else they may already have as a dependency, instead.
If we do this, then I think this abstract class is no longer necessary.

@marcio-absmartly The problem is that we current don't have ContextDataSerializer and ContextEventDeserializer since we need to suport Serialize and Deserialize both. You think worth to create it only for this Cache purposes?

marcio-absmartly · 2023-03-27T09:48:55Z

core-api/src/main/java/com/absmartly/sdk/cache/SqliteCache.java

+
+	private Connection getConnection() throws SQLException {
+		if (this.connection == null) {
+			this.connection = DriverManager.getConnection("jdbc:sqlite:absmarly.db");


Should this file name be a parameter?

marcio-absmartly · 2023-03-27T09:56:48Z

core-api/build.gradle

@@ -21,6 +29,9 @@ dependencies {

 	implementation group: "com.fasterxml.jackson.core", name: "jackson-databind", version: jacksonVersion
 	implementation group: "com.fasterxml.jackson.datatype", name: "jackson-datatype-jsr310", version: jacksonDataTypeVersion
+	implementation group: 'org.xerial', name: 'sqlite-jdbc', version: sqliteVersion
+
+	implementation files('../libs/simpleCircuitBreaker-2.0.5.jar')


Are we going to distribute this in our repository?
Can we find something on maven-central or any other repository?

Unfortunately, the developer has not released it to any Maven Repo :(

The developer didn't answer that. Let keep this way until I get a response from them.

marcio-absmartly · 2023-03-27T10:20:33Z

core-api/src/main/java/com/absmartly/sdk/ContextEventHandler.java

@@ -8,4 +8,5 @@

 public interface ContextEventHandler {
 	CompletableFuture<Void> publish(@Nonnull final Context context, @Nonnull final PublishEvent event);
+	void flushCache();


This seems a bit out of place. The "cache" should be an implementation detail, right?

marcio-absmartly · 2023-03-27T10:26:37Z

core-api/src/main/java/com/absmartly/sdk/Context.java

@@ -149,7 +150,12 @@ public Context waitUntilReady() {
 		if (data_ == null) {
 			final CompletableFuture<Void> future = readyFuture_; // cache here to avoid locking
 			if (future != null && !future.isDone()) {
-				future.join();
+				future.whenComplete(new BiConsumer<Void, Throwable>() {


I don't think this should be here for three reasons:

It seems like an implementation detail of the caching mechanism.

What if the user never calls waitUntilReady? i.e. the user may create the context with an already retrieved contextData.

It could be done independently when the eventHandler is created, or instead, we could have a notification-like method in the interface, for example contextBecameReady() or something like that, that is called on the ContextEventHandler interface when it gets ready (not just here).

Did a change here, check if this place I put the code can have some side affect I not see.

hermeswaldemarin added 2 commits March 7, 2023 00:08

Implementing Resilience

1c09e84

Implementing Resilience

63d59a8

marcio-absmartly requested changes Mar 27, 2023

View reviewed changes

hermeswaldemarin added 4 commits April 4, 2023 20:32

Implementing Resilience

56d705e

Implementing Resilience

e279d01

Implementing Resilience

77dc8bf

Implementing Resilience

0441c3e

Implementing Resilience #3

Are you sure you want to change the base?

Implementing Resilience #3

Uh oh!

Conversation

hermeswaldemarin commented Mar 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!